Add clock-aligned waterfall time markers - #5538

🔵 Needs a closer look

The unresolved font-dependent atlas-cache issue and rendering breadth require human review before approval.

    if (!m_wfTimeMarkerAtlasDirty && labels == m_wfTimeMarkerLabels
        && m_wfTimeMarkerAtlas.size() == pixels
        && m_wfTimeMarkerAtlas.devicePixelRatio() == dpr) {

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

1. Issue fit

Yes. Issue #5537 lists eight behaviours and the diff covers all eight: the submenu's nine intervals map exactly onto kWaterfallMarkerIntervals with the labels the issue names; default Off with per-pan-slot persistence; thin line plus left-edge UTC stamp; clock-boundary alignment with the boundary time as the label (boundaryMs = (t / intervalMs) * intervalMs, not arrival time); rows followed through live scroll, pause, history scrub and resize; two theme tokens; both GPU and software render paths; and the preference stored in the nested Display document rather than a flat key.

I also checked the three "review checkpoints" the triage comment on #5537 asked for, and all three hold:

1. Ring re-laid from origin 0 on rebuild — rebuildWaterfallViewportForFrame resets m_wfVisibleTimeRows alongside m_wfWriteRow = 0 (SpectrumWidget.cpp:6129), and paintWaterfallRowsFromHistory re-lays it with the same waterfallVisibleRowForAge(writeRowOrigin, age, height) mapping it uses for pixels, so markers cannot shear away from their signal rows.
2. History scrub refills from m_wfHistoryTimestamps — done at SpectrumWidget.cpp:5988-5991 using m_wfHistoryOffsetRows + age, matching the rowIndex lookup on the line above it.
3. GPU markers get their own quad — drawWaterfallTimeMarkersGpu draws from a dedicated dynamic VBO and does not touch m_overlayStatic / m_overlayStaticDirty, so smooth scroll does not force a full-overlay repaint plus upload.

2. Scope

Everything in the diff is explained by #5537. No unrelated files, no formatting churn, no deleted guards — I read the - lines and the only removals are the context lines the hunks re-emit. CHANGELOG.md correctly untouched.

New public surface, flagged for a maintainer ruling rather than waved through: a Q_PROPERTY(int waterfallTimeMarkerSeconds) on SpectrumWidget; two automation-snapshot keys (waterfallTimeMarkerSeconds, waterfallTimeMarkers); a waterfallTimeMarkers key inside the Display settings document; two theme tokens; and the waterfallTimeMarkersMenu / waterfallTimeMarkers<N> object names. All of it is what the feature needs, and #5537 is labelled maintainer-review with "Final visual/UX direction is subject to maintainer review", so this is a decision to make, not a violation.

Preference check (§6): this adds a new overlay defaulting to Off; it does not restyle, reorder or re-default any existing UI. Not smuggled preference.

Socket tests: neither new test opens, binds, listens or connects — waterfall_time_markers_test links Qt6::Core only and drives the pure geometry function; waterfall_time_marker_settings_test links aethercore + Qt6::Core and touches only the settings store. No fake radio/amp/tuner peer added, and none removed. Nothing to record beyond that.

3. Blockers

None. I tried hard to find one — see §5 for the specific attacks and why each failed.

4. Nits (all non-blocking)

• Test settings isolation hand-rolls the sandbox. tests/waterfall_time_marker_settings_test.cpp:10 sets AETHER_SETTINGS_DIR directly instead of using TestSettingsProfile.h, which the CMake contract names as the isolation mechanism. Inline comment has the detail; it is genuinely lower-risk than it sounds and there is precedent, so a nit.
• Two assertions in that test are tautological (lines 31-34) — the getter validates independently of the setter, so they pass even if the setter wrote garbage. Inline comment.
• m_wfVisibleTimeRows is the only restored vector without a post-restore size re-init (SpectrumWidget.cpp:6821) — currently unreachable, but the parity gap is worth a comment for the next editor. Inline.
• The atlas-caching comment slightly overstates the code (SpectrumWidget_TimeMarkers.cpp:85) — the label dedup depends on marker.y, so row motion can invalidate the atlas. Rare at ≥15 s intervals. Inline.
• QAccessibleValueChangeEvent on the SpectrumWidget (SpectrumWidget_TimeMarkers.cpp:22) is an odd fit — the widget is not a value-type accessible, and the checkable menu actions already expose the selection. Not wrong, just probably not doing anything for a screen reader.
• The 30-minute and 1-hour entries will rarely place a visible line. Already raised in the #5537 triage: kWaterfallHistoryMs caps retention and the visible ring is only height rows, so at typical rates a screenful is well under a minute and an hour boundary lands inside it only occasionally. This matches WSJT-X and is harmless — flagging only because it is a maintainer call on menu space, not a defect.
• Neither new test is on the frozen per-PR ctest gate. ci.yml's -R steps cover the four DV tests, cross_needle_meter_test, mac_nr_filter_test and asr_gpu_probe_test. The five green checks here compiled the new code but did not execute either new test; they run in full-suite.yml at merge. Correct per canon — noting it so "CI is green" is not read as "the new tests passed in CI".

5. What I tried to break (and failed)

Everything below is reasoned from reading the code in /tmp/aetherclaude/pr-5538. I have no build and no way to drive the GUI, so nothing here was reproduced at runtime — where I say a path is safe, I mean I traced every writer, not that I watched it run.

• Out-of-bounds read on the new ring. appendVisibleRow reads m_wfVisibleTimeRows[m_wfWriteRow] before the pre-existing m_wfWriteRow = (m_wfWriteRow - 1 + h) % h normalizes it — and that modulo is itself the tell that the old code did not assume m_wfWriteRow < h. I traced every reallocation of m_waterfall: resetCurrentWaterfallRowsForSize ends in clearCurrentWaterfallRows() which sets m_wfWriteRow = 0 (:6556); the resize commit sets it to 0 (:11808); rebuildWaterfallViewportForFrame sets it to 0 (:6129); and restoreCurrentWaterfallStreamState bails to the reset branch on stateHasWrongWaterfall before ever assigning restored.wfWriteRow. The invariant holds at entry, so the unnormalized index is in range. This was my best blocker candidate and it does not survive.
• Stale timestamp latch. m_wfIncomingTimestampMs is a latch, so I checked all three appendVisibleRow call sites — the Flex native-tile loop (:8751), the fallback path (:12350) and the Kiwi path (:12407). Each is immediately preceded by appendHistoryRow, and the latch assignment is the first statement in appendHistoryRow, ahead of the hidden-source early returns. No path reaches appendVisibleRow with a stale stamp.
• Markers invisible when the time-scale strip is hidden. The width is std::min(wfContentRect.width(), waterfallTimeScaleRect(wfRect).left() - wfRect.left()), which goes negative if that rect is ever null. waterfallTimeScaleRect is unconditional and anchored to wfRect.right(), so left() - wfRect.left() is always wfRect.width() - stripWidth. No negative-width clip.
• GPU SRB layout incompatibility with the borrowed m_ovPipeline. The new SRB has one binding; m_ovSrb, which the pipeline was created against, also has exactly one (sampledTexture(1, FragmentStage, …)) — layout-compatible, so setShaderResources is legal.
• The #3294 premultiply-squared class of bug. The atlas is Format_RGBA8888_Premultiplied and m_ovPipeline's blend is srcColor = One / dstColor = OneMinusSrcAlpha — the correct pairing for a premultiplied source. Had it been SrcAlpha, the translucent label backing would have been crushed exactly as the passband fill was.
• Scroll-offset sign, i.e. markers sliding against their rows during animation. m_waterfallScrollDistanceRows - waterfallScrollProgressRows() is byte-identical to the pre-existing sampleOffsetRows at SpectrumWidget.cpp:15699, which is what drawWaterfall's sourceStart uses. Same convention, not a re-derivation.
• Duplicate/wrong labels from the m_wfTimeMarkerLabels.indexOf(...) lookup. For a monotonic run, two visible markers cannot share a boundary bucket (the younger row's predecessor would already be inside it, so it would not register a crossing). The one non-monotonic case — an NTP step back that re-crosses the same boundary — yields two markers with the same correct label, which is what indexOf renders.
• Degenerate geometry. The line quad's zero-height source rect, an empty labels list, and zero-width clips: the clipped.isEmpty() early return precedes every division, atlasHeight is ≥ 2 even with no labels, and labels is non-empty whenever markers is (the first marker always clears the dedup threshold).
• Token hex byte order. #dc0f0f1a is 9 chars, so Qt reads #AARRGGBB — alpha 220 over #0f0f1a. colorToTokenString emits HexArgb for alpha < 255, and #5a0f0f1a already ships in default-dark.json with the same RGB. The value follows the house convention rather than inventing one.
• Whether the new tests would pass against a broken implementation. waterfall_time_markers_test is real: I hand-evaluated the wrapped-ring case (head 3, size 4, offset 0.25, height 40) and the asserted y values 7.5 and 27.5 and the count of 2 all follow from the guards. Deleting the timestampMs <= previousMs check breaks "a batched tile must not repeat a timestamp"; deleting the bucket comparison breaks "before boundary"; returning arrival time instead of the boundary breaks the per-interval delayed-row case. It does not assert the implementation back to itself.
• Could not check: any runtime/visual behaviour, the actual GPU draw on a real RHI backend, and whether the 1 px line reads well against a live waterfall. The PR body's macOS ARM64 automation-bridge verification is not something I can reproduce headless — a maintainer eyeballing the overlay in both render paths (and at DPR 2) would settle the visual half.

6. Recommendation

Needs maintainer decision — on the feature itself, not on the code. #5537 carries maintainer-review and explicitly reserves visual/UX direction, and this PR adds new public surface (a Q_PROPERTY, two automation keys, a settings key, two theme tokens). On engineering grounds I would merge it: it sits on structures that already existed rather than adding a retention layer, the geometry is a pure function with a genuine test behind it, both render paths are covered, the per-slot scope correctly avoids the global propagation the triage warned about, and it survived every attack in §5. The two open calls are whether the 30-minute and 1-hour entries earn their menu space, and a visual check of the overlay in both paths. Nits are all cosmetic or test-hygiene and none need to block.

Nice work — this is unusually careful about the ring/scroll invariants, which is where this feature had the most room to go quietly wrong.

_{🤖 aethersdr-agent · cost: $14.2168 · model: claude-opus-5}